Skip to content

Conversation

@Valariael
Copy link

For each element created or modified by the function, we test :

  • if it exists,
  • its position,
  • the value of its attributes,

before and after modification.
In three cases :

  • with an empty array,
  • an array of one picture,
  • an array of two pictures.

Copy link
Contributor

@jacekkopecky jacekkopecky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly just polishing

package.json Outdated
"multer": "^1.3.0",
"mysql2": "^1.5.1"
"mysql2": "^1.5.1",
"node-fetch": "^1.7.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't need to be there

@@ -0,0 +1,173 @@
'use strict';

fetch = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please include a comment on what we're doing here and why - hiding the original fetch

const elM = document.querySelector('main');
assert.strictEqual(
elM.childElementCount,
(0+baseMainChildrenNumber),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might not need the (0+ ... ) bits here

);
assert.ok(
document.querySelector('main .picture') == null,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every assertion should have a message


assert.strictEqual(
el.length,
(0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need () - basically everywhere please

let images = as[0].querySelectorAll('img');
let deletes = section.querySelectorAll('div');

assert.ok(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this cannot fail

'The `<a>` element must have its `href` attribute set to the good value.',
);

assert.strictEqual(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a blank line before this block of tests, maybe add another line with a comment like

// test the image inside the a

images = as[0].querySelectorAll('img');
deletes = section.querySelectorAll('div');

assert.strictEqual(listOfSections.length, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to actually repeat the messages from above because if any of these assertions fail, the developer won't be told why, what's going on

@Valariael Valariael force-pushed the putpicturesinpage-test branch 2 times, most recently from 2fd380b to ffc96df Compare April 25, 2018 09:09
@Valariael Valariael force-pushed the putpicturesinpage-test branch from ffc96df to 48ee7ad Compare April 25, 2018 09:26
@jacekkopecky
Copy link
Contributor

Very good, thank you! We'll decide later how we'll use the PR on this repo. Please don't delete your fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants